Closed Bug 776290 Opened 12 years ago Closed 12 years ago

Function.toSource() changed its output in Nightly07/21

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
17 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: addon-compat) 

Function.toSource() changed its output in Nightly07/21

This brakes many userChrome.js scripts which change code by using Function.toSource().
And maybe break some extensions also).

Step To Reproduce:

For Example:
function aaa(){
  if (a || /*cond1*/ b) {
    var c;
  }
}
aaa.toSource();

Actual Rresult:

Before landing Bug 761723, the output is :
function aaa() {if (a || b) {var c;}}

After landing Bug 761723, the output is :
function aaa(){
  if (a || /*cond1*/ b) {
    var c;
  }
}

So, eval("aaa = " + aaa.toSource().replace("(a || b)", "(a || b || c)")
does not work as expected.


Expected Result:
  Function.toSource() should not change its output
Keywords: addon-compat
This is expected. Writing code like that is already brittle; it could easily break if the code is altered.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
For the record, I owe anyone responsible for breaking such code a beer. Two beers if they break it badly.
Such source-patching code would also fail if someone edited the original function being patched, to rework the logic or rename a variable. I agree that this isn't a problem that needs to be addressed by toSource.
Should we really be hosting add-ons that do this kind of thing? It seems to me that add-on code review should flag such maneuvers.
We allow it under some conditions because there are circumstances where the alternative is too complex.
(In reply to Jim Blandy :jimb from comment #4)
> Should we really be hosting add-ons that do this kind of thing?

I have strong opinions on that, which is why I'm happy about any changes that make it untenable. Unfortunately, we allow it by policy now.
Blocks: 776718
Any alternative way to get normalized source of functions for debugging?
Another use case of Function.prototype.toString() was inspecting "how the code was parsed by Firefox". For example,

    function doSomething() {
      if (condition1)
        if (condition2)
           return 'a';
      else
        return 'b';
    
      throw new Error('unexpected case');
    }

This code was written expecting it throws an error when the condition1 is true and the condition2 is false, like:

    function doSomething() {
      if (condition1) {
        if (condition2) {
           return 'a';
        }
      } else {
        return 'b';
      }
    
      throw new Error('unexpected case');
    }

But Firefox actually parses it as:

    function doSomething() {
      if (condition1) {
        if (condition2) {
           return 'a';
        } else {
          return 'b';
        }
      }
    
      throw new Error('unexpected case');
    }

On old Firefox I could debug such mistakes in my addon because old Firefox output normalized source of functions like the last example, via toString() (indented version), toSource() (without indent) or uneval().
However, currently all of toString(), toSource() and uneval() return the original source. As the result, I cannot know how the "broken" code was parsed by Firefox anymore.
Not in Firefox, but I hope any good JS beautifer will do this for you.
(In reply to Benjamin Peterson from comment #8)
> Not in Firefox, but I hope any good JS beautifer will do this for you.

I think such solution can solve simple JS syntax errors, but doesn't help debugging of Firefox addons. My point is "how the code is parsed by Firefox". Sorry, the example in the comment #7 was not suitable for the topic.

Actually, Firefox introduces changes around JS engine on each release. Even if an addon works correctly on an old Firefox, it can be broken on a new Firefox. Then, I usually open the error console and inspect why the feature doesn't work. On such debugging process, it is very important that "how it is parsed" by the specific version Firefox.
(In reply to SHIMODA Hiroshi from comment #9)
> Actually, Firefox introduces changes around JS engine on each release. Even
> if an addon works correctly on an old Firefox, it can be broken on a new
> Firefox. Then, I usually open the error console and inspect why the feature
> doesn't work. On such debugging process, it is very important that "how it
> is parsed" by the specific version Firefox.

Can you give us a real-life example of a case where looking at the result of toSource helped you debug a problem? We do change the JS engine, but changes to the way we interpret a given piece of source code ought to be very rare.

You could construct a tool based on Reflect.parse to show how Firefox parsed a given piece of code. Reflect.parse takes JS source and returns a parse tree made of ordinary JS objects. For example:

js> Reflect.parse("if (a) print('b');")                                        
({loc:{start:{line:1, column:0}, end:{line:1, column:18}, source:null}, type:"Program", body:[{loc:{start:{line:1, column:0}, end:{line:1, column:17}, source:null}, type:"IfStatement", test:{loc:{start:{line:1, column:4}, end:{line:1, column:5}, source:null}, type:"Identifier", name:"a"}, consequent:{loc:{start:{line:1, column:7}, end:{line:1, column:17}, source:null}, type:"ExpressionStatement", expression:{loc:{start:{line:1, column:7}, end:{line:1, column:17}, source:null}, type:"CallExpression", callee:{loc:{start:{line:1, column:7}, end:{line:1, column:12}, source:null}, type:"Identifier", name:"print"}, arguments:[{loc:{start:{line:1, column:13}, end:{line:1, column:16}, source:null}, type:"Literal", value:"b"}]}}, alternate:null}]})
(In reply to Jim Blandy :jimb from comment #10)
> Can you give us a real-life example of a case where looking at the result of
> toSource helped you debug a problem? We do change the JS engine, but changes
> to the way we interpret a given piece of source code ought to be very rare.

Sorry, I misunderstood the point. Most problems I saw are not caused by changes of JS engine. Errors from such changes are already reported in the error console with a link to the source code. We don't have to see function's live source on the case.

> You could construct a tool based on Reflect.parse to show how Firefox parsed
> a given piece of code. Reflect.parse takes JS source and returns a parse
> tree made of ordinary JS objects. For example:

Oh, I didn't know it.
https://developer.mozilla.org/en/SpiderMonkey/Parser_API
And I also found a decompiler for its output on the bug 590755.

Anyway, the old toSource() which generates *normalized*, *executable* JS code fragment without too much information was useful for some cases, not only monkey patching. Real-life example for me:

 * Generating bookmarklet.
   If a function contains one line comment (started with "//"),
   it is hard to be converted to a bookmarklet because such comments
   can break following codes when all lines are joined to one line.
   toSource() can convert such a function to a one liner safely.
 * Inspecting parsed result of functions in human readable format.
   toSource() was enough for this case.
   (The result of Reflect.parse() seems not human readable.)

If some convenient shorthand like "Reflect.stringify(Reflect.parse( function.toSource()), { normalize: true})" is available on Firefox,
it can be enough for my purposes.
(In reply to Jim Blandy :jimb from comment #3)
> Such source-patching code would also fail if someone edited the original
> function being patched, to rework the logic or rename a variable. I agree
> that this isn't a problem that needs to be addressed by toSource.

Even if monkey patching is fragile, I think that it can be done more safer in some cases. There are some tips to do it safely (For example, "insert codes just after '{'", and so on.) Normalized sources help us doing safer patching. Complete pairs of "(", ")", "{" and "}" are good landmarks to insert code blocks without breaking original code paths. And, of course "comment-free" is also important.

Please separate "should not" and "can not". We addon authors "should not" do
monkey patching, but in some cases it is actually required as told in
the comment #5. By this change (now we have no way to get normalized sources
fromlive functions), we "can not" do it safely on Firefox 17. In other words, patchings become more dangerous than old Firefox.

And, even if it become dangerous, I still use the way *just for me*, because
features I required cannot be implemented without monkey patching. And,
because I don't use other unknown addons, I don't have to take care of conflict with unknown addons. Even if patching fails, *I* can read sources of both addons
and I'll fix it by myself. However, users *not me* can use other unknown addons.
They cannot fix conflict themselves. In the end, who gets the short end of the stick? End users.
(In reply to SHIMODA Hiroshi from comment #12)
> They cannot fix conflict themselves. In the end, who gets the short end of
> the stick? End users.

I'm sorry it's causing trouble for you, but after reading your examples it still seems to me the benefit to developers in general (that is, all people writing or debugging code on Firefox) outweighs the problems introduced for patchers.

I've added a comment to bug 590755 saying that the serializer would be more valuable than we thought. If you could advance the patches in that bug, that would help the work proceed.
(In reply to SHIMODA Hiroshi from comment #12)
Another way forward would be to submit patches to the code you patch at present, changing them to instead have APIs that extensions can use.
This is a regression.
Firefox's friendliness to add-on developers is greatly reduced.
So Jorge, how many extensions on addons.mozilla.org use toSource() to monkey patch Firefox and how many will be broken when autobumped?
It's not a regression, it's a feature which works as intended and is meant to be (and is) considerably more friendly to add-on developers.

Philip: There are more of them than I'd like there to be, but add-ons that do this should not be auto-bumped in any case, and won't be if bug 776718 ever gets fixed.
(In reply to Philip Chee from comment #17)
> So Jorge, how many extensions on addons.mozilla.org use toSource() to monkey
> patch Firefox and how many will be broken when autobumped?

It's hard to tell how many use it, but my guess is that it'll be at least a few dozen. Not all of them are necessarily broken with this change, though, it depends on how their validations are done.

(In reply to Kris Maglione [:kmag] from comment #18)
> Philip: There are more of them than I'd like there to be, but add-ons that
> do this should not be auto-bumped in any case, and won't be if bug 776718
> ever gets fixed.

It doesn't matter if they're not autobumped, since Firefox will assume they're compatible anyway. The benefit of the compatibility bump process is that we can warn the developers with (hopefully) sufficient time to fix it.
(In reply to Kris Maglione [:kmag] from comment #18)
> It's not a regression, it's a feature which works as intended and is meant
> to be (and is) considerably more friendly to add-on developers.

The purpose of Bug 761723 is to save source. This is a side effect, isn't it?
No. Saving the source was the method, not the outcome. The purpose was to save the original source to be returned by toString.
I've found an interesting thing around the source code of the JavaScript engine.

http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#5214

> 5214 /*
> 5215  * API extension: OR this into indent to avoid pretty-printing the decompiled
> 5216  * source resulting from JS_DecompileFunction{,Body}.
> 5217  */
> 5218 #define JS_DONT_PRETTY_PRINT    ((unsigned)0x8000)

http://mxr.mozilla.org/mozilla-central/source/js/src/jsfun.cpp#746

> 746 static JSBool
> 747 fun_toString(JSContext *cx, unsigned argc, Value *vp)
> 748 {
> 749     CallArgs args = CallArgsFromVp(argc, vp);
> 750     JS_ASSERT(IsFunctionObject(args.calleev()));
> 751 
> 752     uint32_t indent = 0;
> 753 
> 754     if (args.length() != 0 && !ToUint32(cx, args[0], &indent))
> 755         return false;
> 756 
> 757     JSObject *obj = ToObject(cx, args.thisv());
> 758     if (!obj)
> 759         return false;
> 760 
> 761     JSString *str = fun_toStringHelper(cx, obj, indent);
> 762     if (!str)
> 763         return false;
> 764 
> 765     args.rval().setString(str);
> 766     return true;
> 767 }
> 768 
> 769 #if JS_HAS_TOSOURCE
> 770 static JSBool
> 771 fun_toSource(JSContext *cx, unsigned argc, Value *vp)
> 772 {
> 773     CallArgs args = CallArgsFromVp(argc, vp);
> 774     JS_ASSERT(IsFunctionObject(args.calleev()));
> 775 
> 776     JSObject *obj = ToObject(cx, args.thisv());
> 777     if (!obj)
> 778         return false;
> 779 
> 780     JSString *str = fun_toStringHelper(cx, obj, JS_DONT_PRETTY_PRINT);
> 781     if (!str)
> 782         return false;
> 783 
> 784     args.rval().setString(str);
> 785     return true;
> 786 }
> 787 #endif

As above, "toSource()" of JavaScript functions was defined as "don't pretty print" intentionally, and it was the point different from "toString()". However, actually now "toSource()" returns the value just same to "toString()". I think this is actually a side effect of changes introduced by the bug 761723. (Note: some add-on developers will be happy if there is a way to get "not pretty printed" source. In old Firefox, "toSource()" and "uneval()" were the answer.)
No one follow this after comoment 22? It breaks tons of things and make addon developers misery
(In reply to dindog from comment #24)
> No one follow this after comoment 22? It breaks tons of things and make
> addon developers misery

I'm sorry, but WONTFIX is still the right answer to this bug.

For JavaScript developers in general, being able to provide the original source for the code present in the system is a huge improvement.

For those of us who work on the JavaScript engine itself, the decompiler was a frequent source of problems, and it took effort to maintain. Removing the decompiler will make our work easier. 

Retaining the original source may also help us to implement some optimizations, making Firefox faster for its users.

However, keeping add-on developers happy is also important to us. I do not agree with the feelings expressed in comment 2: I do not want to make existing, necessary kludges more difficult without a good reason.

Note that the change to toString/toSource has not made code patching impossible. Add-on developers can still patch code by editing the source and re-evaluating, when there is no better alternative. However, since the results of toString/toSource now reflect all changes to the source code --- changes to spacing, parentheses, and comments --- the regular expressions an add-on uses to find the code to patch will break more frequently. Note that this is an existing problem: the decompiler-generated code changed sometimes, too, if the original code changed enough.

As a separate problem, the one-time change from decompiled source to original source may have broken many existing regular expressions, as the decompiled source probably rarely matched the original exactly. However, this is a one-time problem: once the regular expressions have been updated, they will remain valid until the particular section of code they edit is changed.

So, before commenting in this bug to say that your code patching has been affected, please ask yourself which category you are in:

1) If your code patch was affected simply by the change from decompiled source to original source, then this is a one-time difficulty which should be easy to overcome: simply update the regular expression to properly match the current sources.

2) If your code patch applies to a line of source code which frequently has significant changes made to it, then the decompiler probably wasn't protecting you from those anyway, and you simply have a high-maintenance code patch. In this case, we have not made things much worse than they already were.

3) If your code patch applies to a line of source code that rarely changed under the decompiler, but changes frequently now that you have the original source, then I consider that a much more reasonable complaint.

However, I believe that 3) is rare, and that most troubles fall into category 1).
(In reply to SHIMODA Hiroshi from comment #22)
> As above, "toSource()" of JavaScript functions was defined as "don't pretty
> print" intentionally, and it was the point different from "toString()".
> However, actually now "toSource()" returns the value just same to
> "toString()". I think this is actually a side effect of changes introduced
> by the bug 761723. (Note: some add-on developers will be happy if there is a
> way to get "not pretty printed" source. In old Firefox, "toSource()" and
> "uneval()" were the answer.)

I think we need to rename the flag, and fix the documentation; I have filed bug 789768. I suspect that this is not the answer you were looking for; I apologize.
So Function.toSource() === Function.toString() now?
Yes.
Blocks: 813772
Hello, I don't agree with the status REOLVED WONTFIX. Why? I'll explain...

Functions toString() and toSource() are APIs used in possibly hundreds of addons. If you change the API to behave differently without something like a "compilation warning or error" you'll get big problems. Look for example at Java. If they change the API, they tag the old functions as @Deprecated and introduce new ones.

Can't you do the same? If an add-on uses the deprecated API, just make an entry in the error console that the add-on uses deprecated function. If you need new features similar to what the toSource function did, make a new function /like toSourceCode()/. This wouldn't break old code and the new feature will work too.

So please change again the status and fix it, because your approach is not even close to professional. And I know what I'm talking about.

;-)
Addons should have a compatibility warning for 17 about toSource. See bug 776718.
(In reply to Michal Stybnar from comment #30)
> Hello, I don't agree with the status REOLVED WONTFIX. Why? I'll explain...
> 
> Functions toString() and toSource() are APIs used in possibly hundreds of
> addons.

We checked before changing the API and are aware of the extent of the impact.

> If you change the API to behave differently without something like a
> "compilation warning or error" you'll get big problems. Look for example at
> Java. If they change the API, they tag the old functions as @Deprecated and
> introduce new ones.

That is generally our policy.  In this specific case, the existence of the prior implementation of toSource was actively holding back ES6 feature work and several large-scale optimizations.  Given that the new functionality is largely equivalent to the prior functionality, more closely matches other browsers, and had relatively small usage, we saw this as the best approach reachable with our current resources.
Nothing was deprecated. No interface was changed to the extent that it no longer matched its documented behavior. There would be no way to issue a deprecation warning, since those methods can't be renamed and then removed. In any case, the behavior that this broke is by nature fragile, is long deprecated (in the dictionary sense) by pretty much anyone you ask, and has always been highly prone to breakage from release-to-release to the extent that the somewhat larger scale breakage caused by this change is as far as I'm concerned par for the course.

In any case, this is a done deal. It's moved through the trains and landed in release. There's no backing it out now, so there's no point in further debate.
(In reply to Kris Maglione [:kmag] from comment #33)
> Nothing was deprecated. No interface was changed to the extent that it no
> longer matched its documented behavior.

The interface was changed in at least one extent - the most significant. The function returns something absolutely different than in previous versions.

> In any case, the behavior that this broke is by nature fragile, is long
> deprecated (in the dictionary sense) by pretty much anyone you ask, and has
> always been highly prone to breakage from release-to-release to the extent
> that the somewhat larger scale breakage caused by this change is as far as
> I'm concerned par for the course.

Well this was written in a little overcomplicated manner. :-) I don't argue about a behavior, it's fragile nature or security. It's known and the "security concern" should have been long ago mentioned in the error console to explicitly say to the developers that this is "prone to breakage from release-to-release". That's how it's done in any other APIs - but it seems that this is ignored here in Mozilla.

> In any case, this is a done deal. It's moved through the trains and landed
> in release. There's no backing it out now, so there's no point in further
> debate.

I was not checking Nightly a few months now. That's why I reacted now when some addons stopped working. Anyway the problem itself is explained already by you. It should never have landed into any release before previous deprecation of the old functionality! That's what I'm talking about.
You need to log in before you can comment on or make changes to this bug.